Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat: Set onbeforeunload to avoid losing the current terminal session with Ctrl+W #23

Merged
merged 2 commits into from
Oct 29, 2024
Merged

feat: Set onbeforeunload to avoid losing the current terminal session with Ctrl+W #23

merged 2 commits into from
Oct 29, 2024

Conversation

erikmd
Copy link
Contributor

@erikmd erikmd commented Sep 14, 2024 •
edited
Loading

Description

As an Emacs user (and co-maintainer of several packages), I extensively use Gitpod Terminal Browser in multiple occasions. Gitpod Terminal is a very nice tool!
— even if the README states that "it is not ready for production use", I guess it's pretty close 👍

Motivation of this PR: given the Ctrl+W is a very typical Emacs shortcut that must be avoided when using Gitpod (because it amounts to stopping the workspace and losing the ambient session), this small feature appears to be very important for Gitpod+Terminal users.

BTW I followed advice from https://developer.mozilla.org/en-US/docs/Web/API/Window/beforeunload_event:

The dialog can be triggered in the following ways:

  • Calling the event object's preventDefault() method.
  • Setting the event object's returnValue property to a non-empty string value or any other truthy value.
  • Returning any truthy value from the event handler function, e.g. return "string". Note that this only works when the function is attached via the onbeforeunload property, not the addEventListener() method. This behavior is consistent across modern versions of Firefox, Safari, and Chrome.

The last two mechanisms are legacy features; best practice is to trigger the dialog by invoking preventDefault() on the event object, while also setting returnValue to support legacy cases.

Then tested the feature successfully on Firefox ESR (under Debian Stable).

Cc @filiptronicek @mustard-mh @loujaybee @axonasif WDYT?

Related Issue(s)

Fixes #gitpod-io/gitpod#18416

How to test

Documentation

/hold

erikmd added a commit to erikmd/xterm-web-ide that referenced this pull request Sep 14, 2024
@erikmd
fix: validate.sh was incomplete for testing gitpod-io#23
3910d09 
`./validate.sh` currently fails on a fresh gitpod workspace with:

```
rm: cannot remove '/ide/xterm': Is a directory
ln: failed to create symbolic link '/ide/xterm/xterm': File exists
```
@axonasif
Copy link
Member

Thanks for the efforts @erikmd!

We'll take a look at this 👋

@mustard-mh
Copy link

@filiptronicek could you take a look? 🙏

@filiptronicek
Copy link
Member

Hey there @erikmd 👋, I took the liberty to push to your PR to make this configurable on the user level with the XTERM_CONFIRM_BROWSER_EXIT environment variable. If it's set (and it can be set on all workspaces you ever start with user-scoped environment variables), it will prompt you to confirm before actually closing the Browser Terminal.

Below is a demo (left is with XTERM_CONFIRM_BROWSER_EXIT=true, right is without):

Recording 2024-10-29 at 12 55 51

@filiptronicek filiptronicek merged commit d547d4f into gitpod-io:main Oct 29, 2024
@erikmd erikmd deleted the feat-gitpod-18416 branch October 29, 2024 12:06
@erikmd
Copy link
Contributor Author

erikmd commented Oct 29, 2024

OK @filiptronicek, thanks a lot! 👍

Side-question: will xterm-web-ide be reused in Gitpod Flex at some point? or it is strongly related to Gitpod Classic.

@filiptronicek
Copy link
Member

@erikmd because of the architectural differences between Classic and Flex, the editor will definitely not be able to be used as-is.

That said, because of just how Flex-ible Flex is designed to be, there definitely will be (and to some degree already are) extension points for a slightly modified Xterm IDE. AFAIK, you can actually use Automations today to run the IDE for every environment you create (see the postEnvironmentStart hook). This will require a bit of tinkering though since we only officially release the IDE for now as a Classic Docker image.

@erikmd
Copy link
Contributor Author

erikmd commented Oct 29, 2024

OK, thanks @filiptronicek for your feedback!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@erikmd @axonasif @mustard-mh @filiptronicek